Validate flags/positionals before the commands/nodes that own them - #629
Validate flags/positionals before the commands/nodes that own them#629hexonal wants to merge 1 commit into
Conversation
Context.Validate() iterated c.Path in trace order and called each element's Validate() as encountered. Since a command's own Path entry is appended before its flags' entries (flags are parsed after the command name in the token stream), a command's Validate() ran before its flags' Validate() - so a command validator couldn't rely on its own flags already being validated. A node's own flags/positionals always appear on the path right after that node, before the next node entry. So each node/command's own validation is now deferred until that boundary (or the end of the path) is reached, running it right after its own flags/positionals rather than before them. This only reorders each node relative to its own directly- owned flags; the relative order between different nodes on the path (e.g. a parent command still validates before its child command) is unaffected. Fixes alecthomas#611.
|
@alecthomas — a heads-up that this PR and #630 have no CI results, and it isn't because anything failed. Both sit at Since neither the
Both are regression fixes against open issues — #629 fixes #611 (command validated before its own flags), #630 fixes #489 ( No rush on reviewing; the only thing that needs you specifically is the CI approval, so the results are visible here rather than on my machine. |
1 participant
Fixes #611.
Bug
Context.Validate()iteratesc.Pathin trace order and calls each element'sValidate()as it's encountered. Since a command's ownPathentry is appended before its flags' entries (flags come later in the token stream, e.g.command --flag value), the command'sValidate()ran before its flags'Validate()— so a command's validator couldn't rely on its own flags already being validated:Fix
A node's own flags/positionals always appear on the path contiguously right after that node, before the next node entry. So each node's own validation is now deferred (via a single
pendingNodevariable) until that boundary — or the end of the path — is reached, running it right after its own flags/positionals instead of before them.This only reorders each node relative to its own directly-owned flags. The relative order between different nodes on the path is unaffected — a parent command still validates before its child command, exactly as before. (An earlier version of this fix used a flat two-pass split over the whole path instead, which had a wider side effect: an ancestor command's validation could get delayed by an unrelated descendant command's flags. This version avoids that by only ever holding back the most recently encountered node, not the whole path.)
Known limitation
Resolve()appends resolver-derived flag values (e.g. from a config-fileResolver) to the end ofc.Pathwithout recording which node each one belongs to. Because of that, a node whose flags come from aResolver(rather than the command line) can still end up deferred past other nodes' resolver-supplied flags too — noted in a code comment. Fixing this properly would mean also threading ownership throughResolve()'s appended entries, which felt like more than this fix should take on; happy to follow up separately if that's worth doing.Tests
TestValidateCmdRunsAfterItsFlags— the issue's exact repro (single command, one flag).TestValidateNestedCmdRunsAfterOwnFlagsOnly— two-level nesting (app flag, parent command + its own flag, child command + its own flag), confirming each level's own flag validates immediately before it while app → parent → child ordering is preserved.Both confirmed to fail against pre-fix code and pass with the fix.